ESO-447: Expand Vault e2e for TLS trustedCABundle and ExternalSecret templating - #169

test/e2e/cert_manager_helpers_test.go (1)

109-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer k8serrors.IsNotFound over string matching.

strings.Contains(err.Error(), "not found") is fragile compared to the structured k8serrors.IsNotFound(err) check used elsewhere in this PR (e.g. ensureVaultNamespace). Text matching can silently misclassify errors if the message format changes.

♻️ Proposed fix

 		pods, err := clientset.CoreV1().Pods(certManagerOperandNamespace).List(ctx, metav1.ListOptions{})
 		if err != nil {
-			if strings.Contains(err.Error(), "not found") {
+			if k8serrors.IsNotFound(err) {
 				return false, nil
 			}
 			return false, err
 		}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/cert_manager_helpers_test.go` around lines 109 - 141, Update
waitForCertManagerOperandPods to use the Kubernetes structured not-found check
k8serrors.IsNotFound(err) instead of matching "not found" in err.Error(), while
preserving the existing retry behavior for not-found errors and propagation of
all other errors.

test/e2e/e2e_test.go (1)

2759-2816: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Wrap ConfigMap update with retry.RetryOnConflict.

createVaultCAConfigMap and createSampleCAConfigMap do a plain Get→mutate .Data→Update without conflict retry. The referenced ConfigMap (vaultCAConfigMapName) is the same object the operator's watch-label reconciler patches once it's referenced by trustedCABundle (per trusted_ca_bundle_test.go's watch-label restoration test), so a concurrent operator patch between our Get and Update can produce a conflict error that isn't retried, unlike the established retry.RetryOnConflict pattern used in setTrustedCABundle/clearTrustedCABundle in this same file.

♻️ Proposed fix (apply the same pattern to both functions)

-	existing, err := clientset.CoreV1().ConfigMaps(operandNamespace).Get(ctx, vaultCAConfigMapName, metav1.GetOptions{})
-	if k8serrors.IsNotFound(err) {
-		_, err = clientset.CoreV1().ConfigMaps(operandNamespace).Create(ctx, cm, metav1.CreateOptions{})
-		return err
-	}
-	if err != nil {
-		return err
-	}
-	existing.Data = cm.Data
-	_, err = clientset.CoreV1().ConfigMaps(operandNamespace).Update(ctx, existing, metav1.UpdateOptions{})
-	return err
+	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		existing, err := clientset.CoreV1().ConfigMaps(operandNamespace).Get(ctx, vaultCAConfigMapName, metav1.GetOptions{})
+		if k8serrors.IsNotFound(err) {
+			_, err = clientset.CoreV1().ConfigMaps(operandNamespace).Create(ctx, cm, metav1.CreateOptions{})
+			return err
+		}
+		if err != nil {
+			return err
+		}
+		existing.Data = cm.Data
+		_, err = clientset.CoreV1().ConfigMaps(operandNamespace).Update(ctx, existing, metav1.UpdateOptions{})
+		return err
+	})

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/e2e_test.go` around lines 2759 - 2816, Wrap the existing ConfigMap
Get, Data mutation, and Update flow in both createVaultCAConfigMap and
createSampleCAConfigMap with retry.RetryOnConflict, re-fetching the ConfigMap on
each retry and returning non-conflict errors immediately. Preserve the existing
create-on-NotFound behavior and ensure the retry callback propagates the final
update error.

test/e2e/trusted_ca_bundle_test.go (1)

236-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add failure messages to newly added Gomega assertions. Both sites add new Expect/g.Expect assertions without a descriptive failure message, unlike neighboring assertions in the same PR (e.g. the restoration check a few lines below in the same test) that do include one — this is explicitly called out for **/*_test.go files.

• test/e2e/trusted_ca_bundle_test.go#L236-L238: add a message to g.Expect(cm.Labels).To(HaveKeyWithValue(...)), e.g. "ConfigMap %s should have the watch label after ExternalSecretsConfig is Ready".
• test/e2e/e2e_test.go#L2012-L2014: add messages to Expect(createVaultCAConfigMap(...)).To(Succeed()) and Expect(createSampleCAConfigMap(...)).To(Succeed()), e.g. "failed to create Vault CA ConfigMap" / "failed to create sample CA ConfigMap".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/trusted_ca_bundle_test.go` around lines 236 - 238, Add descriptive
failure messages to all newly added Gomega assertions: update the ConfigMap
label assertion near the ExternalSecretsConfig readiness check in
test/e2e/trusted_ca_bundle_test.go (lines 236-238), and add distinct
creation-failure messages to the createVaultCAConfigMap and
createSampleCAConfigMap assertions in test/e2e/e2e_test.go (lines 2012-2014).

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/e2e/e2e_test.go (3)

2012-2046: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add failure messages to the bare Expect(...).To(Succeed()) assertions.

Lines 2013, 2014, 2027, 2030, 2040 and 2043 assert success without a message. The helper errors carry context, but a message states the intent of each step directly in the failure output. Example: .To(Succeed(), "SecretStore %s should report Ready=False with reason %s", secretStoreResourceName, invalidProviderConfigReason).

As per coding guidelines: "Flag Ginkgo test assertions that lack meaningful failure messages."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/e2e_test.go` around lines 2012 - 2046, Add meaningful failure
messages to each bare Expect(...).To(Succeed()) assertion in this test flow,
including createVaultCAConfigMap, createSampleCAConfigMap, both loader
operations, setTrustedCABundle calls, and the readiness waits. Use messages
describing the intended operation and relevant resource or condition
identifiers, while preserving the existing assertions and cleanup behavior.

Source: Coding guidelines

---

2682-2722: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make the error-handling style of ensureVaultNamespace consistent, and add GinkgoHelper().

The function returns error for the Get and Create paths, but it uses Eventually(...).Should(...) in the terminating-namespace path. A failure in that path aborts the spec through Gomega instead of returning an error to the caller. applyVault calls this function and propagates its error, so the two failure modes behave differently.

Also add GinkgoHelper() at the top so Gomega reports the caller line, as clearTrustedCABundle already does at line 2824.

♻️ Proposed change

 func ensureVaultNamespace(ctx context.Context, clientset *kubernetes.Clientset) error {
+	GinkgoHelper()
 	By(fmt.Sprintf("Ensuring namespace %s exists", vaultNamespace))

Alternatively, replace the two Eventually blocks with wait.PollUntilContextTimeout and return the resulting error, so the helper reports every failure through its return value.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/e2e_test.go` around lines 2682 - 2722, Add GinkgoHelper() at the
start of ensureVaultNamespace, and replace both Eventually(...).Should(...)
blocks in the terminating-namespace branch with context-aware polling that
returns errors. Ensure timeout or polling failures, including namespace deletion
and recreation failures, propagate through ensureVaultNamespace to applyVault
instead of aborting via Gomega.

---

1978-1978: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider splitting this spec into two It blocks.

The spec asserts two behaviors: trustedCABundle validation (failure with the sample CA, recovery with the Vault CA) and secret synchronization (PushSecret, ExternalSecret, target Secret content). The Context is Ordered, so a second It can reuse the state established by the first. A split gives a precise failure signal when only one behavior breaks.

As per coding guidelines: "Review Ginkgo test code for single responsibility: each test (It block) should test one specific behavior."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/e2e_test.go` at line 1978, Split the spec into two ordered It
blocks: keep the non-matching and Vault CA trustedCABundle validation/recovery
assertions together in the first, and move PushSecret, ExternalSecret, and
target Secret synchronization/content assertions into the second. Preserve the
existing Ordered Context state so the second test reuses the successful Vault CA
setup.

Source: Coding guidelines

test/e2e/e2e_test.go (1)

1978-1986: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider splitting this spec by behavior.

This It asserts four behaviors: trustedCABundle rejection, trustedCABundle recovery, PushSecret synchronization, and ExternalSecret synchronization. A failure in the later stages does not identify which behavior broke. The Context is Ordered, so you can move the PushSecret and ExternalSecret stages into separate It blocks and keep the CA switch in the first block.

Also, externalsecretsConfigFile through targetSecretKey are never reassigned. Declare them with const to match the adjacent templating spec.

As per coding guidelines: "Review Ginkgo test code for single responsibility: each test (It block) should test one specific behavior. Flag tests that assert multiple unrelated behaviors".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/e2e_test.go` around lines 1978 - 1986, Split the ordered spec around
its trustedCABundle rejection/recovery, PushSecret synchronization, and
ExternalSecret synchronization behaviors into separate It blocks, keeping the
Vault CA switch in the first block and preserving shared ordered-state setup as
needed. In the spec’s local declarations, change externalsecretsConfigFile,
vaultSecretStoreFile, vaultExternalSecretFile, secretStoreResourceName,
externalSecretResourceName, and targetSecretKey from var entries to const
declarations.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes